-
-
Notifications
You must be signed in to change notification settings - Fork 213
fix: Http client exception not handled properly resulting in incorrectly formatted error #1021
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
fix: Http client exception not handled properly resulting in incorrectly formatted error #1021
Conversation
Thanks for opening this pull request! |
| )); | ||
| } | ||
|
|
||
| if (exception is ClientException) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use two clients, dio and http, at Parse Flutter !
In this change you are using the functions of the http package. Will this not cause any problems when we use the dio package client?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DioException is already being handled above but ClientException was missing.
|
Can you please run CI? |
|
@mbfakourii done |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1021 +/- ##
==========================================
+ Coverage 43.37% 43.47% +0.10%
==========================================
Files 61 61
Lines 3463 3466 +3
==========================================
+ Hits 1502 1507 +5
+ Misses 1961 1959 -2 ☔ View full report in Codecov by Sentry. |
|
I will reformat the title to use the proper commit message syntax. |
|
I rephrased the PR title; not sure whether it accurately describes the issue, could you please review? |
|
I think it's better. |
|
There is still an open review discussion and the changelog entry + version bump is missing in the PR. See other PRs on how they are done. Then we can go ahead and merge. |
📝 WalkthroughWalkthroughThe changes consolidate the SDK's modularized structure and enhance exception handling. The main library file now imports the http package and removes part directives that previously split functionality across separate files. Additionally, ClientException from the http package is now explicitly handled in response exception processing, converting it to a ParseError. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
I thought changelog and version bump should be maintained on release instead of PR merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/dart/lib/parse_server_sdk.dart (1)
12-12: Narrow http import to avoid potential symbol clashes with dioGiven this is the root library for many
partfiles and you already importpackage:dio/dio.dart, bringing in all ofpackage:http/http.dartrisks name clashes (e.g.,Response) in any part that uses unqualified types.Since the only thing needed here is
ClientExceptionforbuildParseResponseWithException, consider narrowing the import:-import 'package:http/http.dart'; +import 'package:http/http.dart' show ClientException;This keeps the namespace clean while still enabling the new exception handling.
Please double‑check that no other symbols from
httpare used anywhere in the parts of this library before applying this change.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/dart/lib/parse_server_sdk.dart(1 hunks)packages/dart/lib/src/objects/response/parse_exception_response.dart(1 hunks)
🔇 Additional comments (1)
packages/dart/lib/src/objects/response/parse_exception_response.dart (1)
25-29: ClientException handling looks correct and aligns with existing patternsThe new
ClientExceptionbranch cleanly maps http client errors into aParseError, consistent with the generic fallback (no explicit error code, message taken from the exception) and separate from the richerDioExceptionhandling above.Functionally this should resolve the issue where
ClientException.toString()was previously exposed.Please confirm:
- That you have a test covering a thrown
ClientExceptionand asserting the resultingParseResponse.error.message.- That no additional metadata (e.g., a specific error code) is needed for
ClientExceptionbeyond what the generic fallback already provided.
Pull Request
Issue
Closes: #1022
Approach
Tasks
Summary by CodeRabbit
Bug Fixes
Refactor